OCPBUGS-95069: agent/waitfor: verify expected nodes before declaring install complete#10711
OCPBUGS-95069: agent/waitfor: verify expected nodes before declaring install complete#10711savio87 wants to merge 4 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@savio87: This pull request references Jira Issue OCPBUGS-95069, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesInstall completion node readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
@savio87: This pull request references Jira Issue OCPBUGS-95069, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hi @savio87. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/openshift-install/command/waitfor.go`:
- Around line 121-183: Add unit tests for
cmd/openshift-install/command/waitfor.go lines 121-183 covering Ready and
NotReady role-labeled nodes, missing master/worker counts, zero expectations,
and error paths for client or node listing; add unit tests for
cmd/openshift-install/agent/waitfor.go lines 128-147 covering expected
master/worker derivation from optional install-config, including control-plane
and multiple compute-pool replicas. Use the existing test conventions and fake
Kubernetes/config clients where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ec9ec5cb-9744-4847-a7a3-25686bdf2517
📒 Files selected for processing (2)
cmd/openshift-install/agent/waitfor.gocmd/openshift-install/command/waitfor.go
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/openshift-install/command/waitfor_node_test.go (1)
70-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the worker-not-Ready branch.
Add a case with two worker nodes where one is
NodeReady=False; currently only the master readiness loop is directly tested for that condition.Proposed test case
+ { + name: "worker node not ready", + nodes: []*corev1.Node{newNode("worker-0", "worker", true), newNode("worker-1", "worker", false)}, + expectedWorkers: 2, + expectErr: true, + errContains: "expected 2 worker node(s) to be Ready but only 1 found", + },As per path instructions, “Verify edge cases are covered, especially for validation and defaulting logic.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/openshift-install/command/waitfor_node_test.go` around lines 70 - 83, Add a wait-for-node test case alongside the existing worker cases with two worker nodes, one Ready and one NodeReady=False, and assert the expected error for an insufficient number of Ready workers. Use the existing node-construction helper and readiness assertions in the test to cover the worker-not-Ready branch.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/openshift-install/command/waitfor_node_test.go`:
- Around line 70-83: Add a wait-for-node test case alongside the existing worker
cases with two worker nodes, one Ready and one NodeReady=False, and assert the
expected error for an insufficient number of Ready workers. Use the existing
node-construction helper and readiness assertions in the test to cover the
worker-not-Ready branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 25e40e88-ac89-40de-bae3-bcd4c5f958a4
📒 Files selected for processing (4)
cmd/openshift-install/agent/waitfor.gocmd/openshift-install/agent/waitfor_test.gocmd/openshift-install/command/waitfor.gocmd/openshift-install/command/waitfor_node_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/openshift-install/agent/waitfor.go
- cmd/openshift-install/command/waitfor.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/openshift-install/agent/waitfor.go`:
- Around line 130-136: Handle errors from assetStore.Load in the install-config
loading flow before processing installConfigAsset: propagate or terminate on
non-optional load failures rather than silently leaving expectedMasters and
expectedWorkers at zero. Preserve the existing optional-config behavior when no
asset is available, and keep the FIPS and extractExpectedNodeCounts handling for
successfully loaded configs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a2dd3216-d0db-42f2-85a9-b1ce3c5aba8a
📒 Files selected for processing (4)
cmd/openshift-install/agent/waitfor.gocmd/openshift-install/agent/waitfor_test.gocmd/openshift-install/command/waitfor.gocmd/openshift-install/command/waitfor_node_test.go
| if installConfigAsset, err := assetStore.Load(&agentasset.OptionalInstallConfig{}); err == nil && installConfigAsset != nil { | ||
| ic := installConfigAsset.(*agentasset.OptionalInstallConfig) | ||
| if ic.Config != nil { | ||
| fipsEnabled = ic.Config.FIPS | ||
| expectedMasters, expectedWorkers = extractExpectedNodeCounts(ic.Config) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently disable node verification on install-config load failures.
Line 130 ignores every Load error, leaving both counts at zero; verifyExpectedNodes then skips validation. Return or terminate on non-optional load errors instead of treating them as an absent config. As per path instructions, “Never ignore error returns.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/openshift-install/agent/waitfor.go` around lines 130 - 136, Handle errors
from assetStore.Load in the install-config loading flow before processing
installConfigAsset: propagate or terminate on non-optional load failures rather
than silently leaving expectedMasters and expectedWorkers at zero. Preserve the
existing optional-config behavior when no asset is available, and keep the FIPS
and extractExpectedNodeCounts handling for successfully loaded configs.
Source: Path instructions
Log a warning instead of silently skipping FIPS and node verification when the install-config cannot be loaded. Co-authored-by: Cursor <cursoragent@cursor.com>
|
coderabbitai full review |
Cover the branch where a worker node exists but has NodeReady=False, mirroring the existing master-not-ready test. Co-authored-by: Cursor <cursoragent@cursor.com>
|
We already have checks in place that should have caught this issue. I think it would be better to improve those checks, rather than making more specific checks for each case, particularly as the more general checks will help with upgrades as well as installs. |
|
Thanks for the feedback, @patrickdillon. I agree that improving the existing general checks would be the better long-term fix -- especially since that would cover upgrades as well as installs. Could you point me to which specific checks you think should be improved? For example, should waitForStableOperators() also verify that operators are not Degraded (not just not Progressing)? That said, I think there may still be value in a node readiness check as a complementary safety net. The general checks rely on operators accurately reporting their status, but as Zane Bitter noted on the Jira issue, there was a ~10-minute window where etcd reported Progressing=False before transitioning to Degraded=True. A direct node count verification would catch the "missing node" case regardless of operator reporting gaps. Happy to take whichever direction you think is best -- whether that's improving the existing checks, keeping this as a complementary safeguard, or both. |
4 participants
Summary
Fixes https://issues.redhat.com/browse/OCPBUGS-95069
Details
The wait-for install-complete command currently only checks ClusterVersion conditions and operator stability. As reported in OCPBUGS-95069, these conditions can transiently be satisfied even when a node (e.g. master-0) was never created, causing the command to falsely report success.
This change follows the same pattern used for FIPS verification (PR #10348) - adding a verification step in WaitForInstallComplete and wiring the expected values from the agent CLI.
Test plan
Summary by CodeRabbit